-
Notifications
You must be signed in to change notification settings - Fork 127
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Blog ideation #643
Blog ideation #643
Conversation
@@ -120,7 +120,7 @@ Commands: | |||
list List all available scripts in workspace | |||
create <name> Create a new script | |||
fix fix all definition files |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The usage of the 'compile' command has been updated to include '<folders...>' but the description 'Compile all script in workspace' has not been updated to reflect the change in usage. Consider revising the description to indicate that the command now accepts specific folder patterns as arguments.
generated by pr-docs-review-commit
command_usage_inconsistency
.map((f) => scriptFolders.find((sf) => sf.dirname === f)) | ||
.filter((f) => f) | ||
|
||
for (const folder of foldersToCompile) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no error handling for the case when the folders
argument is not provided or is an empty array. This could lead to unexpected behavior or errors. Consider adding error handling or a default value. 🚧
generated by pr-review-commit
missing_error_handling
|
||
if (responseType === undefined) { | ||
text = unfence(text, "(markdown|md)") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function unfence
is called only when responseType
is undefined. However, there is no handling for the case when responseType
is not undefined. This could lead to unexpected behavior. Consider adding handling for this case. 🚧
generated by pr-review-commit
unhandled_case
@@ -323,6 +323,7 @@ export async function runTemplate( | |||
trace.detailsFenced(`📁 file ${fn}`, content) | |||
const fileEdit = await getFileEdit(fn) | |||
fileEdit.after = content | |||
fileEdit.validation = { valid: true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The valid
property of fileEdit.validation
is hardcoded to true
. This might not reflect the actual validation status of the file edit. Consider calculating this value based on the actual validation results. 🚧
generated by pr-review-commit
hardcoded_value
The code changes seem to be valid from the TypeScript compiler's stand point. Here are some key points:
Overall, these changes are enhancing the functionality of the project and giving users more control. However, you may want to test the change in maximum tool calls extensively to ensure it doesn't degrade performance. So, LGTM 🚀
|
@@ -164,10 +164,13 @@ Options: | |||
### `scripts compile` | |||
|
|||
``` | |||
Usage: genaiscript scripts compile [options] | |||
Usage: genaiscript scripts compile [options] [folders...] | |||
|
|||
Compile all script in workspace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There seems to be a typo here. It should be "Compile all scripts in workspace" instead of "Compile all script in workspace".
generated by pr-docs-review-commit
typo
|
||
Compile all script in workspace | ||
|
||
Arguments: | ||
folders Pattern to match files |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The description "Pattern to match files" for the argument 'folders' is unclear. It should specify that 'folders' is a pattern to match directories containing scripts to compile.
generated by pr-docs-review-commit
description_error
.option( | ||
"-rr, --run-retry <number>", | ||
"number of retries for the entire run" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing semicolon
generated by pr-review-commit
missing_semi
@@ -205,6 +208,7 @@ export async function cli() { | |||
scripts | |||
.command("compile") | |||
.description("Compile all script in workspace") | |||
.argument("[folders...]", "Pattern to match files") | |||
.action(compileScript) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing semicolon
generated by pr-review-commit
missing_semi
@@ -120,7 +120,7 @@ Commands: | |||
list List all available scripts in workspace | |||
create <name> Create a new script | |||
fix fix all definition files |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usage description for 'compile' command is inconsistent with the updated usage syntax.
generated by pr-docs-review-commit
incorrect_usage
@@ -164,10 +164,13 @@ Options: | |||
### `scripts compile` | |||
|
|||
``` | |||
Usage: genaiscript scripts compile [options] | |||
Usage: genaiscript scripts compile [options] [folders...] | |||
|
|||
Compile all script in workspace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Description for 'compile' command is incomplete, missing the explanation for the new '[folders...]' argument.
generated by pr-docs-review-commit
incorrect_description
.option( | ||
"-rr, --run-retry <number>", | ||
"number of retries for the entire run" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function cli
does not validate the input arguments. This could lead to unexpected behavior or errors if the function is called with invalid or unexpected arguments. It's a good practice to validate function arguments to ensure they match the expected format and type.
generated by pr-review-commit
missing_argument_validation
@@ -205,6 +208,7 @@ export async function cli() { | |||
scripts | |||
.command("compile") | |||
.description("Compile all script in workspace") | |||
.argument("[folders...]", "Pattern to match files") | |||
.action(compileScript) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function cli
does not validate the input arguments for the compile
command. This could lead to unexpected behavior or errors if the function is called with invalid or unexpected arguments. It's a good practice to validate function arguments to ensure they match the expected format and type.
generated by pr-review-commit
missing_argument_validation
compile
command for GenAI scripts has been broadened to incorporate multiple folder input, enhancing its flexibility. Now you can specify which folders to compile using this command. In absence of any arguments, all scripts in the workspace will be compiled, as it functioned before.unfence
, has been added to 'fence.ts'. This function removes the fence that identifies a particular language section in a text.MAX_TOOL_CALLS
constant in 'constants.ts' has been ramped up from 100 to 10,000. This upgrade to higher value hints at supporting robust, resilient and complex tool interactions.name
has been added for storing a short name of the test.